WrapIterImpl: add support for inputs with line breaks #123

axelchalon · 2018-01-18T16:39:47Z

Closes #122
does the trick but it needs reviewing!

mgeisler · 2018-01-20T21:52:41Z

Hi Alex, thanks for the PR! I'm about to go to the US for 10 days so please don't be afraid if I don't get around to comment on their before February :-)

My first thought was that this is something the caller could handle by splitting the input string on newlines and then mapping fill over each line. It'll of course be less efficient, so maybe this is why you want it in line?

mgeisler · 2018-01-20T21:54:40Z

By the way, the build failure seems to be caused by a breaking change in a dependency - not because of your changes. I'll have to sort that out...

axelchalon · 2018-01-22T14:01:07Z

Hi Martin! Sure, no worries!

Yes, you're right that we could split the input first and then call fill on each line; however, I think it'd be more efficient if this was taken care of inside of fill.

Also, currently, calling fill on an input with line breaks yields unexpected results (i.e. output has line breaks at what seems at first like random positions, #122), so even if we don't decide that fill preserve line breaks, then I imagine they should probably be at least stripped out so that we have a consistent result.

Have a nice trip!

mgeisler · 2018-02-05T18:31:35Z

Hi Alex, I'm back again :-)

I think you're right: the current behavior is confusing... I had actually expected fill to convert \n into a space since the splitting is done on whitespace, not just on . However, as you noticed, the \n is kept in the output and so weird line breaks show up!

I've considered adding a refill method (see #60) which would try to be "intelligent" and detect paragraphs in the input text. I would expect such a function to require a blank line between paragraphs -- similarly to how Markdown separates paragraphs and how people have been formatting plaintext emails for decades. The reason: it would allow you to wrap the text nicely in your source file, while still have the text adapt to the user's terminal on display.

Do you think we should put that kind of logic into fill in all cases? That is, turn a single \n into a and turn \n\n+ into what your patch treats like a single paragraph break?

Perhaps it's better to keep fill simple and just fix the annoying line breaks... a more fancy re-wrapping refill function could then be added on top. I would like a refill function to handle things like bullet lists too (more or less the same way Emacs handles these things out of the box).

mgeisler

I don't like how complex this function is already, so I tried to see if we could avoid adding more code than absolutely necessary here.

Please give it a try and also correct the other minor things. Thanks!

mgeisler · 2018-02-05T18:32:17Z

src/lib.rs

+
+    #[test]
+    fn multiline() {
+        assert_eq!(fill("1 3 5 7\n1 3 5 7",11),"1 3 5 7\n1 3 5 7");


Could you run rustfmt on the file? I believe it would insert a space after the ,. Thanks!

mgeisler · 2018-02-05T18:40:14Z

src/lib.rs

+
+                    self.start = self.split + self.split_len;
+                    self.line_width += wrapper.subsequent_indent.width();
+                    self.line_width -= self.line_width_at_split;


I believe this is redundant -- in total, self.line_width becomes wrapper.subsequent_indent.width() since you set it to self.line_width_at_split above. So you can remove this line and simplify the previous line to a direct assignment:

self.line_width = wrapper.subsequent_indent.width();

mgeisler · 2018-02-05T18:43:18Z

src/lib.rs

+    fn multiline() {
+        assert_eq!(fill("1 3 5 7\n1 3 5 7",11),"1 3 5 7\n1 3 5 7");
+        assert_eq!(fill("1 3 5 7\n1 3 5 7",5),"1 3 5\n7\n1 3 5\n7");
+        assert_eq!(fill("1 3 5 7\n1 3 5 7",6),"1 3 5\n7\n1 3 5\n7");


It's a little hard for me to figure out which corner-case each test is testing :-) Could you perhaps add multiple smaller tests if there is a common theme?

Here, I think a third line with

assert_eq!(fill("1 3 5 7\n1 3 5 7", 7), "1 3 5 7\n1 3 5 7");

could be interesting -- it would demonstrate that the code doesn't accidentally add an extra \n.

mgeisler · 2018-02-05T18:45:29Z

src/lib.rs

+        assert_eq!(fill("test\nabcdefghi\n",11),"test\nabcdefghi\n");
+        assert_eq!(fill("test\nabcdefghi\n\n",11),"test\nabcdefghi\n\n");
+        assert_eq!(fill("test\nabcdefghijklmnopq\n\n",11),"test\nabcdefghijk\nlmnopq\n\n");
+        assert_eq!(fill("test\nabcdefghijklmnopq abcdefghijk\n\n",11),"test\nabcdefghijk\nlmnopq\nabcdefghijk\n\n");


The length of "abcdefghijklmnopq" is 17, so I'm not actually sure if there is a relation between that number and the width of 11?

mgeisler · 2018-02-05T18:49:03Z

src/lib.rs

+
+                // If this is not the final line, return the current line. Otherwise,
+                // we will return the line with its line break after exiting the loop
+                if self.split + self.split_len < self.source.len() {


I didn't actually try, but would it be possible to combine the logic here with the existing logic that kicks in when the current line it too long? Maybe by changing the conditions slightly:

if ch != '\n' && is_whitespace(ch) { // ... } else if ch == `\n` || self.line_width + char_width > wrapper.width { // ... }

No idea if it works! :-D

hmm maybe but I can't really seem to get it working this way ^^ feel free to commit if you find a way

axelchalon · 2018-02-08T15:46:58Z

Thanks for the feedback! I'm a bit busy at the moment but I'll get back to you in a bit

mgeisler · 2018-02-12T23:14:54Z

Hi @axelchalon, no stress at all! Let me know if you think my suggestions are annoying or unhelpful, in which case I'll be happy to do them myself. Otherwise just take your time to iterate on this.

I should have fixed the brokenness of master by bumping the minimum version of Rust we test against. If you rebase your branch and force push, you should see tests passing.

axelchalon · 2018-02-19T15:39:48Z

Hey Martin! It's all good, no worries :)

I would expect such a function to require a blank line between paragraphs [...] The reason: it would allow you to wrap the text nicely in your source file, while still have the text adapt to the user's terminal on display.

Hmm, Rust already supports escaping line breaks and subsequent leading whitespace with \ at the end of a line. Also, I would expect such issues to be handled by the language itself, and I feel that libraries shouldn't make perhaps less intuitive decisions to make the language more easy-to-use? I guess we can count on the rust dev team to keep working on this?
I understand where you're coming from, but I think the library should stick to its primary purpose; I see the library more as a util library for quick text reformatting / simple text manipulation, not so much as a pretty printer / library for markdown-ey parsing of human-formatted input. As such I would expect the library to execute minimal tasks and for its functions to do a single thing (and do it well! :D). I would expect the fill function to simply wrap the input (and keep the line breaks). For example if I want to wrap the following string (the help message of a CLI option):

Set the operating mode. MODE can be one of:
    last - Uses the last-used mode, active if none;
    active - Parity continuously syncs the chain;
    passive - Parity syncs initially, then sleeps and wakes regularly to resync;
    dark - Parity syncs only when the RPC is active;
    offline - Parity doesn't sync.",

It certainly wouldn't feel intuitive for me to have to separate each line with an extra blank line. I would expect the function to simply do what it's supposed to do: wrap the lines that are too long. (Of course, we could say that this function could handle lists as well, but then again it would overreach its simple purpose / intended function.)

mgeisler · 2018-02-20T16:07:13Z

I understand where you're coming from, but I think the library should stick to its primary purpose; I see the library more as a util library for quick text reformatting / simple text manipulation, not so much as a pretty printer / library for markdown-ey parsing of human-formatted input.

Very good point! More elaborate wrapping logic should at the very least be put into a different function then.

mgeisler · 2018-02-20T16:33:54Z

Thanks a lot, I've merged the branch and will make a release in one of the next days.

axelchalon mentioned this pull request Jan 18, 2018

Wrap --help output to 120 characters openethereum/parity-ethereum#7626

Merged

mgeisler requested changes Feb 5, 2018

View reviewed changes

mgeisler changed the title ~~[fill] Add support for inputs with line breaks~~ WrapIterImpl: add support for inputs with line breaks Feb 5, 2018

axelchalon added 4 commits February 19, 2018 16:16

Add support for inputs with line breaks

2f2b20f

Run rustfmt

025a71b

Remove cow_add_assign

5fc1e20

Remove redundant assignment

158db92

axelchalon force-pushed the master branch from 14d0e8f to 158db92 Compare February 19, 2018 15:40

Split up tests

2a122d9

mgeisler approved these changes Feb 20, 2018

View reviewed changes

mgeisler merged commit 157c1e7 into mgeisler:master Feb 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WrapIterImpl: add support for inputs with line breaks #123

WrapIterImpl: add support for inputs with line breaks #123

axelchalon commented Jan 18, 2018

mgeisler commented Jan 20, 2018

mgeisler commented Jan 20, 2018

axelchalon commented Jan 22, 2018

mgeisler commented Feb 5, 2018

mgeisler left a comment

mgeisler Feb 5, 2018

mgeisler Feb 5, 2018

mgeisler Feb 5, 2018

mgeisler Feb 5, 2018

mgeisler Feb 5, 2018

axelchalon Feb 20, 2018

axelchalon commented Feb 8, 2018

mgeisler commented Feb 12, 2018

axelchalon commented Feb 19, 2018

mgeisler commented Feb 20, 2018

mgeisler commented Feb 20, 2018

WrapIterImpl: add support for inputs with line breaks #123

WrapIterImpl: add support for inputs with line breaks #123

Conversation

axelchalon commented Jan 18, 2018

mgeisler commented Jan 20, 2018

mgeisler commented Jan 20, 2018

axelchalon commented Jan 22, 2018

mgeisler commented Feb 5, 2018

mgeisler left a comment

Choose a reason for hiding this comment

mgeisler Feb 5, 2018

Choose a reason for hiding this comment

mgeisler Feb 5, 2018

Choose a reason for hiding this comment

mgeisler Feb 5, 2018

Choose a reason for hiding this comment

mgeisler Feb 5, 2018

Choose a reason for hiding this comment

mgeisler Feb 5, 2018

Choose a reason for hiding this comment

axelchalon Feb 20, 2018

Choose a reason for hiding this comment

axelchalon commented Feb 8, 2018

mgeisler commented Feb 12, 2018

axelchalon commented Feb 19, 2018

mgeisler commented Feb 20, 2018

mgeisler commented Feb 20, 2018